Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fixed panic when DSN="memory" is configured #574

Merged
merged 7 commits into from
Jul 15, 2020
Merged

fix: Fixed panic when DSN="memory" is configured #574

merged 7 commits into from
Jul 15, 2020

Conversation

tricky42
Copy link
Contributor

Related issue

If you configured Kratos to use DSN=memory Kratos crashed with a panic as the code used the registry before it was initialized properly.

Proposed changes

Ensure auto migrate logic for DSN=memory is executed after Registry has been initalized successfully.

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Excuting the migration logic in registry.go cause a panic as the registry is not initalized at that point. Therefore we decided to move the handling to driver_default.go, after the registry has been initalized.
@aeneasr
Copy link
Member

aeneasr commented Jul 13, 2020

I don't understand why CircleCI doesn't trigger for you...

Changed SQLite string used for shortcut "memory" to "sqlite://:memory:?_fk=true"
Adjusted tests
Minor fix to Makefile: changed docker image name to include -sqlite in tag for target docker as Kratos is compiled with SQLite support.
Added test case "memory" to e2e tests
Adjusted run.sh to only call kratos migrate sql if test case != "memory
@tricky42
Copy link
Contributor Author

@aeneasr @zepatrik implemented the discussed changes, can you take another look at it?

driver/driver_default.go Outdated Show resolved Hide resolved
@aeneasr aeneasr merged commit 05e55f3 into ory:master Jul 15, 2020
@aeneasr
Copy link
Member

aeneasr commented Jul 15, 2020

Awesome 🎉

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants